Fix thread safety in fast_tanh toggle#178
Conversation
Use atomic<bool> and runtime check instead of modifying shared map.
sdatkinson
left a comment
sdatkinson
left a comment
There was a problem hiding this comment.
Thanks for the PR!
If you can check to make sure that this doesn't clash with the real-time safety of any of the nets, then this seems ok.
Also, some testing to cement the intent of the change would be appreciated.
| { | ||
| if (_activations.find(name) == _activations.end()) | ||
| // Return FastTanh when Tanh is requested and fast_tanh mode is enabled | ||
| if (name == "Tanh" && using_fast_tanh.load(std::memory_order_relaxed)) |
There was a problem hiding this comment.
Does this change make the WaveNet's getting of the sigmoid activation for its gating activation no longer real-time safe?
NeuralAmpModelerCore/NAM/wavenet.cpp
Line 56 in 6ea03b1
|
Thanks for the review! Real-time safety: For non-"Tanh" lookups like WaveNet's "Sigmoid": It just hits the string comparison (false), then goes straight to find() - same as before, actually one fewer map lookup since we reuse the iterator now. For "Tanh" lookups, the atomic load is lock-free with memory_order_relaxed - compiles down to a plain memory read on x86/ARM. One thing I did consider: _activations.at("Fasttanh") technically can throw, though "Fasttanh" is guaranteed to exist. If you'd prefer, I can cache the pointer or use find() instead to be more defensive. Tests: Happy to add tests - what would you like to see? Unit tests for toggle behavior, multi-threaded stress tests, or both? |
2 participants
Summary
Test plan